Track inbound payments by `PaymentId` by tnull · Pull Request #948 · lightningdevkit/ldk-node

tnull · 2026-06-24T15:15:21Z

Closes #298.

This finally switches our inbound payments over to be fully tracked by payment id, and decouples payment IDs from payment hashes.

ldk-reviews-bot · 2026-06-24T15:15:24Z

👋 Thanks for assigning @tankyleo as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

tnull · 2026-07-07T14:58:58Z

Rebased without further changes for now.

tnull · 2026-07-07T15:27:49Z

Force-pushed some fixes:

> git diff-tree -U2  d1b5ffb3 b1f65649
diff --git a/bindings/python/src/ldk_node/test_ldk_node.py b/bindings/python/src/ldk_node/test_ldk_node.py
index 304caf9c..063c95f7 100644
--- a/bindings/python/src/ldk_node/test_ldk_node.py
+++ b/bindings/python/src/ldk_node/test_ldk_node.py
@@ -236,5 +236,5 @@ class TestLdkNode(unittest.TestCase):

         sender_payment = node_1.payment(keysend_payment_id)
-        receiver_payment = node_2.payment(keysend_payment_id)
+        receiver_payment = node_2.payment(received_event.payment_id)

         self.assertIsNotNone(sender_payment)
diff --git a/tests/integration_tests_migration.rs b/tests/integration_tests_migration.rs
index c4e63451..59621476 100644
--- a/tests/integration_tests_migration.rs
+++ b/tests/integration_tests_migration.rs
@@ -204,5 +204,5 @@ async fn migrate_node_across_all_backends() {
 	let invoice = node_b.bolt11_payment().receive(10_000, &description.into(), 3600).unwrap();
 	let ln_send_id = node.bolt11_payment().send(&invoice, None).unwrap();
-	expect_payment_successful_event!(node, Some(ln_send_id), None);
+	expect_payment_successful_event!(node, ln_send_id, None);
 	expect_payment_received_event!(node_b, 10_000);

@@ -212,5 +212,5 @@ async fn migrate_node_across_all_backends() {
 	let invoice = node.bolt11_payment().receive(5_000, &description.into(), 3600).unwrap();
 	let ln_receive_id = node_b.bolt11_payment().send(&invoice, None).unwrap();
-	expect_payment_successful_event!(node_b, Some(ln_receive_id), None);
+	expect_payment_successful_event!(node_b, ln_receive_id, None);
 	expect_payment_received_event!(node, 5_000);

joostjager · 2026-07-15T14:11:36Z

Looking at this through the lens of self-imposed versus intrinsic complexity, I think this PR again makes the cost of our current architecture clear. LDK and ldk-node persist related state independently, without a consistent commit boundary, so we have to recover consistency through careful ordering, replay, cleanup, and expiry logic.

tankyleo

A few nits feel free to squash will take a better pass tomorrow

The switch to tracking payments by ID happened with LDK Node v0.3.0, which is >1.5 years old by now. We can be pretty certain that nobody is upgrading from an older version to the upcoming v0.8. Here we hence make the `payment_id` fields in `Event` required which is a nice API simplification that will also be utilized in the next commit. Co-Authored-By: HAL 9000

Gate manual claiming for unknown custom-hash BOLT11 payments behind a config flag. Nodes that do not opt in fail these payments back without storing or queueing user events. Co-Authored-By: HAL 9000

tankyleo · 2026-07-29T01:16:31Z

+	///
+	/// **Warning:** Enabling this may let any holder of a valid invoice generated by this node tie
+	/// up inbound HTLC slots and grow the persisted event queue until the payment is claimed,
+	/// failed, or times out.


I think it's worth highlighting that this could happen after the payment got claimed. In that case, the payment should be failed back instead of claimed twice.

Perhaps even suggest that the user should keep state tracking all pending payment hashes, and fail any payment hashes not in the pending list ?

Adjusted docs in fixup.

Don't yet see a note about this here do you think it's not worth it ?

Don't yet see a note about this here do you think it's not worth it ?

Hmm, we generally have the guidance that users that enable this need to handle PaymentClaimable and fail anything they're not going to claim? Or maybe I'm confused on what specifically you want mentioned?

See thread below

tankyleo · 2026-07-29T01:20:52Z

+	/// [`Config::manually_claim_unknown_bolt11_payments`] is enabled. Enabling this may let any
+	/// holder of a valid invoice generated by this node tie up inbound HTLC slots and grow the
+	/// persisted event queue until the payment is claimed, failed, or times out.


I'd add some language similar to this after this section:
"This event could be emitted multiple times for a single hash, including well after the invoice got claimed, in case this invoice gets paid multiple times. Users should claim the first, and fail the rest."

Adjusted docs in fixup.

Likewise here, do you find it's not necessary ?

See above: users should generally fail anything they're not going to claim. Whether or not they claim doubly-sent payments is a bit up to them, though it's of course nicer not to do so.

Note that if we give this guidance we probably also want to implement this for LDK Server, which would mean yet-another persisted store, meaning we should then probably still do it in LDK Node if we deem it that important. But we just switched away from making this PR overly complex with all the secondary indexes etc. 😬

Any case, I now added a note.

Hmm ok to confirm the plan in ldk-server would be to claim all the hashes we now the preimage for, regardless of duplicates ?

My intuition there says the responsibility is on the sender of the payment to not pay an invoice twice, so seems good to me.

Note that if we give this guidance we probably also want to implement this for LDK Server, which would mean yet-another persisted store, meaning we should then probably still do it in LDK Node if we deem it that important. But we just switched away from making this PR overly complex with all the secondary indexes etc. 😬

Perhaps then we should drop the "should be failed back" note, and just clarify things like this:

"If the same payment hash is paid more than once, claiming each event accepts each payment"

Hmm ok to confirm the plan in ldk-server would be to claim all the hashes we now the preimage for, regardless of duplicates ?

No, the plan would be to either not expose PaymentClaimable at all, or, if we do, still have it be the user's responsibility to handle the events.

My intuition there says the responsibility is on the sender of the payment to not pay an invoice twice, so seems good to me.

Yeah, it is, but it's also not great to knowingly steal money, even if the other party offers it through a mistake. So preferably we would have default behavior that avoids it, though in this case I'm not sure if it's worth the complication.

Use a name that reflects both possible user actions for unknown BOLT11 payments: either claim them with a preimage or fail them back. Co-Authored-By: HAL 9000

Create inbound BOLT11 records from claimable and claimed events so inbound payments can be tracked by the IDs emitted by LDK. Generate outbound BOLT11 IDs from KeysManager entropy instead of deriving them from the payment hash. Co-Authored-By: HAL 9000

Keep outbound BOLT11 payments keyed by payment hash so repeated attempts to pay the same invoice can still return DuplicatePayment. Co-Authored-By: HAL 9000

Clarify that v0.7-or-earlier for-hash invoices can still emit PaymentClaimable after upgrade regardless of the new config flag. Co-Authored-By: HAL 9000

tankyleo · 2026-08-03T22:59:00Z

+			return (payment_id, Some(info));
+		}
+
+		if legacy_id != payment_id {


Codex:

[P2] Migrate current refund records to the event payment ID _ /home/ubuntu/ldk-node/src/event.rs:691-694
When request_refund_payment is used, it still inserts the inbound refund at PaymentId(payment_hash.0) (src/payment/bolt12.rs:444-464). This fallback therefore treats a current record as legacy and returns the hash-derived ID, so refund PaymentReceived events and store entries never use LDK's
unique inbound ID; repeated refund payments also collapse onto the same record. Restrict the fallback to genuinely legacy rows or re-key this record when the event arrives.

Should we drop inserting the payment into the store on request_refund_for_payment ?

Hmm, good point. Issue is that we can't easily drop the insertion in request_refund_for_payment as otherwise we lose payer_note and quantity for inbound payments (the case where at least the former matters). Let's discuss offline whether we could carry them in Bolt12RefundContext.

tankyleo · 2026-08-03T23:07:43Z

+	///
+	/// **Warning:** Enabling this may let any holder of a valid invoice generated by this node tie
+	/// up inbound HTLC slots and grow the persisted event queue until the payment is claimed,
+	/// failed, or times out.


Don't yet see a note about this here do you think it's not worth it ?

tankyleo · 2026-08-03T23:08:41Z

+	/// [`Config::manually_claim_unknown_bolt11_payments`] is enabled. Enabling this may let any
+	/// holder of a valid invoice generated by this node tie up inbound HTLC slots and grow the
+	/// persisted event queue until the payment is claimed, failed, or times out.


Likewise here, do you find it's not necessary ?

Clarify that claiming duplicate PaymentClaimable events accepts each payment, so unexpected duplicate or late payments should be failed. Co-Authored-By: HAL 9000

Cover manually claiming and failing BOLT11 for-hash invoices that were created before upgrading, so the new ID-based APIs keep handling legacy payment-hash IDs. Co-Authored-By: HAL 9000

Avoid adding a direct test dependency for legacy payment hash types when the v0.7 ldk-node crate already re-exports the exact crate used by its API. Co-Authored-By: HAL 9000

tankyleo · 2026-08-05T01:02:42Z

Thanks feel free to squash the existing fixups

tnull force-pushed the 2026-06-payment-id-prefactors branch from 51dba72 to ff3345d Compare June 24, 2026 15:21

tnull marked this pull request as draft June 24, 2026 15:22

tnull force-pushed the 2026-06-payment-id-prefactors branch from ff3345d to 544e100 Compare June 24, 2026 15:23

tnull commented Jun 24, 2026

View reviewed changes

Comment thread src/payment/bolt11.rs

This was referenced Jun 25, 2026

Bump BDK, address pending payment store follow ups #937

Merged

Add splice RBF support #888

Merged

Consider simplifying/refactoring PendingPaymentStore #950

Open

tnull force-pushed the 2026-06-payment-id-prefactors branch from 544e100 to 4c3f2e4 Compare July 2, 2026 13:45

tnull marked this pull request as ready for review July 2, 2026 13:46

ldk-reviews-bot requested a review from tankyleo July 2, 2026 13:47

tnull removed the request for review from tankyleo July 2, 2026 13:50

tnull marked this pull request as draft July 2, 2026 13:50

tnull force-pushed the 2026-06-payment-id-prefactors branch from 4c3f2e4 to 4a24ee7 Compare July 2, 2026 14:08

tnull requested a review from tankyleo July 2, 2026 14:09

tnull marked this pull request as ready for review July 2, 2026 14:09

joostjager mentioned this pull request Jul 6, 2026

Payment Persistence Gap Matrix #966

Open

tankyleo reviewed Jul 7, 2026

View reviewed changes

Comment thread src/payment/bolt11.rs Outdated

Comment thread src/payment/bolt11.rs Outdated

Comment thread src/event.rs Outdated

Comment thread src/payment/bolt11.rs

tnull force-pushed the 2026-06-payment-id-prefactors branch from 4a24ee7 to d1b5ffb Compare July 7, 2026 14:58

tnull force-pushed the 2026-06-payment-id-prefactors branch from d1b5ffb to b1f6564 Compare July 7, 2026 15:27

tankyleo reviewed Jul 7, 2026

View reviewed changes

tnull force-pushed the 2026-06-payment-id-prefactors branch 3 times, most recently from 046355b to 452c643 Compare July 15, 2026 13:19

tnull requested a review from tankyleo July 15, 2026 13:19

tankyleo reviewed Jul 22, 2026

View reviewed changes

Comment thread src/payment/pending_payment_store.rs Outdated

Comment thread src/data_store.rs Outdated

Comment thread src/data_store.rs Outdated

tnull mentioned this pull request Jul 22, 2026

Include onchain transactions in events #448

Open

tankyleo reviewed Jul 22, 2026

View reviewed changes

Comment thread src/event.rs Outdated

Comment thread src/data_store.rs Outdated

Comment thread src/data_store.rs Outdated

Comment thread src/event.rs Outdated

Comment thread src/event.rs

tnull mentioned this pull request Jul 27, 2026

Fix funding-payment reclassification downgrade and deep-reorg duplication #962

Open

tnull force-pushed the 2026-06-payment-id-prefactors branch from 452c643 to f96d289 Compare July 28, 2026 12:47

tnull requested a review from tankyleo July 28, 2026 12:48

Track unknown BOLT11 manual claims

31eb068

Gate manual claiming for unknown custom-hash BOLT11 payments behind a config flag. Nodes that do not opt in fail these payments back without storing or queueing user events. Co-Authored-By: HAL 9000

tnull force-pushed the 2026-06-payment-id-prefactors branch from f96d289 to 748143c Compare July 28, 2026 13:20

tankyleo reviewed Jul 29, 2026

View reviewed changes

tnull added 4 commits July 29, 2026 11:15

f Rename unknown BOLT11 config flag

e6f1526

Use a name that reflects both possible user actions for unknown BOLT11 payments: either claim them with a preimage or fail them back. Co-Authored-By: HAL 9000

f Restore BOLT11 duplicate payments

7d4b84e

Keep outbound BOLT11 payments keyed by payment hash so repeated attempts to pay the same invoice can still return DuplicatePayment. Co-Authored-By: HAL 9000

f Document legacy for-hash events

6cc849f

Clarify that v0.7-or-earlier for-hash invoices can still emit PaymentClaimable after upgrade regardless of the new config flag. Co-Authored-By: HAL 9000

tnull force-pushed the 2026-06-payment-id-prefactors branch from 748143c to 97b27d4 Compare July 29, 2026 10:00

tnull requested a review from tankyleo July 29, 2026 10:01

tankyleo reviewed Aug 3, 2026

View reviewed changes

tnull added 3 commits August 4, 2026 10:38

f Document duplicate manual claims

31a90da

Clarify that claiming duplicate PaymentClaimable events accepts each payment, so unexpected duplicate or late payments should be failed. Co-Authored-By: HAL 9000

Test v0.7 manual claim upgrades

125fcfe

Cover manually claiming and failing BOLT11 for-hash invoices that were created before upgrading, so the new ID-based APIs keep handling legacy payment-hash IDs. Co-Authored-By: HAL 9000

f Use v0.7 lightning types re-export

1f76c18

Avoid adding a direct test dependency for legacy payment hash types when the v0.7 ldk-node crate already re-exports the exact crate used by its API. Co-Authored-By: HAL 9000

tnull force-pushed the 2026-06-payment-id-prefactors branch from 97b27d4 to 1f76c18 Compare August 4, 2026 09:40

Uh oh!

Conversation

tnull commented Jun 24, 2026

Uh oh!

ldk-reviews-bot commented Jun 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tnull commented Jul 7, 2026

Uh oh!

tnull commented Jul 7, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joostjager commented Jul 15, 2026

Uh oh!

tankyleo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tankyleo commented Aug 5, 2026

Uh oh!

Reviewers

Assignees

Labels

ldk-reviews-bot commented Jun 24, 2026 •

edited

Loading